Reviewable status: 0 of 25 files reviewed, 12 unresolved discussions (waiting on @LucasLvy, @maciejka, and @tdelabro)

src/external_transaction_test.rs line 33 at r8 (raw file):

        signature: TransactionSignature(vec![StarkFelt::ONE, StarkFelt::TWO]),
        nonce: Nonce(stark_felt!("0x1")),
        compiled_class_hash: CompiledClassHash(stark_felt!("0x2")),

Can you define and use a similar macro?

Code quote:

stark_felt!

Reviewable status: 0 of 25 files reviewed, 12 unresolved discussions (waiting on @LucasLvy, @maciejka, and @Yoni-Starkware)

src/external_transaction_test.rs line 33 at r8 (raw file):

For values like 0, 1, 2, 3, that are defined as const, isn't using StarkFelt::ONE, StarkFelt::TWO, and StarkFelt::THREE better?

Reviewable status: 0 of 25 files reviewed, 12 unresolved discussions (waiting on @LucasLvy, @maciejka, and @tdelabro)

src/external_transaction_test.rs line 33 at r8 (raw file):

Right, I meant for general hex values. We use it a lot in the blockifier

Reviewed 3 of 15 files at r1, 1 of 17 files at r8, 4 of 22 files at r9, all commit messages.
Reviewable status: 8 of 32 files reviewed, 15 unresolved discussions (waiting on @LucasLvy, @maciejka, @tdelabro, and @Yoni-Starkware)

src/core.rs line 117 at r9 (raw file):

pub const CONTRACT_ADDRESS_PREFIX: &str = "STARKNET_CONTRACT_ADDRESS";
/// The size of the contract address domain.
pub const CONTRACT_ADDRESS_DOMAIN_SIZE: Felt = Felt::from_hex_unchecked(PATRICIA_KEY_UPPER_BOUND);

Why is it called unchecked? Is it because it can panic?

Code quote:

from_hex_unchecked

src/core.rs line 122 at r9 (raw file):

    NonZeroFelt::from_felt_unchecked(
        CONTRACT_ADDRESS_DOMAIN_SIZE - Felt::from(MAX_STORAGE_ITEM_SIZE),
    )

Consider using the try_from implementation, and panic in case of an error.

Code quote:

pub static L2_ADDRESS_UPPER_BOUND: Lazy<NonZeroFelt> = Lazy::new(|| {
    NonZeroFelt::from_felt_unchecked(
        CONTRACT_ADDRESS_DOMAIN_SIZE - Felt::from(MAX_STORAGE_ITEM_SIZE),
    )

src/core.rs line 388 at r9 (raw file):

        const COMPLIMENT_OF_H160: usize = std::mem::size_of::<Felt>() - H160::len_bytes();

        let bytes = felt.to_bytes_be();

Out of curiousity- why use be and not le?

Code quote:

        let bytes = felt.to_bytes_be();

Reviewed 2 of 15 files at r1, 4 of 17 files at r8, 18 of 22 files at r9, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @LucasLvy, @maciejka, @tdelabro, and @Yoni-Starkware)

src/core.rs line 364 at r9 (raw file):

        ClassHash(StarkHash::from_hex($s).unwrap())
    };
}

these macros are used with unsigned integer inputs as well

Code quote:

#[macro_export]
macro_rules! patricia_key {
    ($s:expr) => {
        PatriciaKey::try_from(StarkHash::from_hex($s).unwrap()).unwrap()
    };
}

/// A utility macro to create a [`ClassHash`] from a hex string / unsigned integer representation.
#[cfg(any(feature = "testing", test))]
#[macro_export]
macro_rules! class_hash {
    ($s:expr) => {
        ClassHash(StarkHash::from_hex($s).unwrap())
    };
}

src/external_transaction_test.rs line 33 at r8 (raw file):

not just hex... see my comments on the macros

src/hash.rs line 29 at r9 (raw file):

        Felt::from_hex_unchecked($s)
    };
}

the docstring on this macro was wrong before: this macro is used in tests to convert values to starkfelt in hex or unsigned integer format (or any type T that implements TryFrom for StarkFelt).
This will break many usages as it will only work for string types.
consider adding impl From<T> for StarkFelt blocks for unsigned int / string T types, gated behind #[cfg(any(feature = "testing", test))]

Code quote:

/// A utility macro to create a [`starknet_types_core::felt::Felt`] from a hex string
/// representation.
#[cfg(any(feature = "testing", test))]
#[macro_export]
macro_rules! stark_felt {
    ($s:expr) => {
        Felt::from_hex_unchecked($s)
    };
}

src/hash.rs line 196 at r9 (raw file):

        res.write_all(&self.0[first_index + 1..])?;
        Ok(())
    }

how does this implementation differ from the Felt's implementation of the Serialize trait?
I see the first four bit are used here for felt size, something which is used by papyrus.
@ShahakShama can this logic be moved to papyrus?

Code quote:

    pub fn serialize(&self, res: &mut impl std::io::Write) -> Result<(), Error> {
        // We use the fact that bytes[0] < 0x10 and encode the size of the felt in the 4 most
        // significant bits of the serialization, which we call `chooser`. We assume that 128 bit
        // felts are prevalent (because of how uint256 is encoded in felts).

        // The first i for which nibbles 2i+1, 2i+2 are nonzero. Note that the first nibble is
        // always 0.
        let mut first_index = 31;
        for i in 0..32 {
            let value = self.0[i];
            if value == 0 {
                continue;
            } else if value < 16 {
                // Can encode the chooser and the value on a single byte.
                first_index = i;
            } else {
                // The chooser is encoded with the first nibble of the value.
                first_index = i - 1;
            }
            break;
        }
        let chooser = if first_index < 15 {
            // For 34 up to 63 nibble felts: chooser == 15, serialize using 32 bytes.
            first_index = 0;
            CHOOSER_FULL
        } else if first_index < 18 {
            // For 28 up to 33 nibble felts: chooser == 14, serialize using 17 bytes.
            first_index = 15;
            CHOOSER_HALF
        } else {
            // For up to 27 nibble felts: serialize the lower 1 + (chooser * 2) nibbles of the felt
            // using chooser + 1 bytes.
            (31 - first_index) as u8
        };
        res.write_all(&[(chooser << 4) | self.0[first_index]])?;
        res.write_all(&self.0[first_index + 1..])?;
        Ok(())
    }

src/hash.rs line 218 at r9 (raw file):

        bytes.read_exact(&mut res[first_index + 1..]).ok()?;
        Some(Self(res))
    }

ditto on the serialize comment above @ShahakShama

Code quote:

    pub fn deserialize(bytes: &mut impl std::io::Read) -> Option<Self> {
        let mut res = [0u8; 32];

        bytes.read_exact(&mut res[..1]).ok()?;
        let first = res[0];
        let chooser: u8 = first >> 4;
        let first = first & 0x0f;

        let first_index = if chooser == CHOOSER_FULL {
            0
        } else if chooser == CHOOSER_HALF {
            15
        } else {
            (31 - chooser) as usize
        };
        res[0] = 0;
        res[first_index] = first;
        bytes.read_exact(&mut res[first_index + 1..]).ok()?;
        Some(Self(res))
    }

src/transaction_hash.rs line 156 at r9 (raw file):

// TODO: should be part of core::Felt
pub fn ascii_as_felt(ascii_str: &str) -> Result<Felt, StarknetApiError> {

why was the (crate) dropped?

Code quote:

pub

src/crypto/utils.rs line 20 at r9 (raw file):

#[derive(thiserror::Error, Clone, Debug)]
#[allow(clippy::explicit_auto_deref)]

why are these now needed? curious, non-blocking

Code quote:

#[allow(clippy::explicit_auto_deref)]

Reviewed all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @DvirYo-starkware, @LucasLvy, @maciejka, @noaov1, @tdelabro, @yair-starkware, and @Yoni-Starkware)

.gitignore line 6 at r9 (raw file):

/Cargo.lock
*.DS_Store
.idea/*

We usually don't put "specific develop environment" stuff here. You can configure your own local gitignore by editing the file ~/.gitignore_global

src/core.rs line 12 at r9 (raw file):

use primitive_types::H160;
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use starknet_types_core::felt::{Felt, NonZeroFelt};

Maybe we want to add a type alias type StarkFelt = Felt in order to not break api?

src/core.rs line 364 at r9 (raw file):

Where? I think they shouldn't (I don't think we do that in papyrus)

src/core.rs line 388 at r9 (raw file):

I don't think there was any specific reason

src/hash.rs line 196 at r9 (raw file):

We can define our own StarkHash in the rpc crate if needed
@dan-starkware @yair-starkware @DvirYo-starkware WDYT

src/hash.rs line 218 at r9 (raw file):

Same, but for the client crate

src/block_hash/transaction_commitment.rs line 2 at r9 (raw file):

use starknet_types_core::felt::Felt;
use starknet_types_core::hash::StarkHash;

This is very confusing because there's a StarkHash in sn api. Could you rename this to CoreStarkHash?

src/crypto/patricia_hash.rs line 29 at r9 (raw file):

use bitvec::prelude::{BitArray, Msb0};
use starknet_types_core::felt::Felt;
use starknet_types_core::hash::StarkHash;

Same, rename this.

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @LucasLvy, @maciejka, @noaov1, @ShahakShama, @tdelabro, and @Yoni-Starkware)

src/core.rs line 12 at r9 (raw file):

I prefer not to do that - I would rather the compiler complain everywhere I used StarkFelt, it is now a completely different type.

src/core.rs line 364 at r9 (raw file):

search for stark_felt!, patricia_key! and class_hash! in the blockifier, you will see usages with strings, unsigned integers and even StarkFelts.
this is a useful test util, I see no reason why we shouldn't use it to reduce boilerplate.

src/hash.rs line 196 at r9 (raw file):

I'll unblock this now; @ShahakShama please make sure this code doesn't get lost and is ported to papyrus?

Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @LucasLvy, @maciejka, @noaov1, @ShahakShama, and @Yoni-Starkware)

src/block_hash/transaction_commitment.rs line 2 at r9 (raw file):

starknet_types_core being the new reference for Felt types. Wouldn't it be better to start aliasing the sn-api type rather than this one.
I have been told sn-api is to be deprecated in some time anyway

Reviewable status: all files reviewed, 22 unresolved discussions (waiting on @dorimedini-starkware, @LucasLvy, @noaov1, @ShahakShama, @tdelabro, and @Yoni-Starkware)

.gitignore line 6 at r9 (raw file):

Done.

src/core.rs line 117 at r9 (raw file):

Yes

src/core.rs line 388 at r9 (raw file):

H160::from_slice expects be representation

src/hash.rs line 29 at r9 (raw file):

Ok. Do we want to keep the name? stark_felt does not make sense since the type is now called just Felt.

src/transaction_hash.rs line 35 at r1 (raw file):

Done.

src/transaction_hash.rs line 34 at r3 (raw file):

Done.

src/transaction_hash.rs line 156 at r9 (raw file):

My mistake. Fixed.

src/block_hash/transaction_commitment.rs line 2 at r9 (raw file):

You mean use starknet_types_core::hash::StarkHash as CoreStarkHash; or a hard rename in the types-rs?

In general starknet_types_core::hash::StarkHash is a bad name for something that is not a hash value itself but a something that calculates a hash. It should be a Hasher or HashCalculator but it is a subject for another pr.

Reviewed 6 of 6 files at r10, all commit messages.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @LucasLvy, @maciejka, @noaov1, @ShahakShama, @tdelabro, and @Yoni-Starkware)

src/crypto/utils.rs line 20 at r9 (raw file):

still curious but not as much lol
and please remove the newline between the #[derive] and the enum

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @LucasLvy, @maciejka, @noaov1, @ShahakShama, @tdelabro, and @Yoni-Starkware)

src/hash.rs line 29 at r9 (raw file):

good point. maybe make_felt? or just felt?

Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @LucasLvy, @maciejka, @noaov1, @tdelabro, and @Yoni-Starkware)

src/hash.rs line 104 at r10 (raw file):

/// The StarkNet [field element](https://docs.starknet.io/documentation/architecture_and_concepts/Hashing/hash-functions/#domain_and_range).
#[derive(Copy, Clone, Eq, PartialEq, Default, Hash, Deserialize, Serialize, PartialOrd, Ord)]
#[serde(try_from = "PrefixedBytesAsHex<32_usize>", into = "PrefixedBytesAsHex<32_usize>")]

We need to add this to starknet-types-core

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @LucasLvy, @maciejka, @noaov1, @tdelabro, and @Yoni-Starkware)

src/hash.rs line 196 at r9 (raw file):

We'll move it to papyrus

src/block_hash/transaction_commitment.rs line 2 at r9 (raw file):

You mean use starknet_types_core::hash::StarkHash as CoreStarkHash
yes

I

Reviewable status: 14 of 32 files reviewed, 17 unresolved discussions (waiting on @dorimedini-starkware, @LucasLvy, @noaov1, @ShahakShama, @tdelabro, and @Yoni-Starkware)

src/core.rs line 72 at r3 (raw file):

Done.

src/hash.rs line 29 at r9 (raw file):

I prefer just felt.
I made it more generic. The cost of implementing it at the starknet-api level and not types-rs are two extra imports:

use crate::hash::{FeltConverter, TryIntoFelt};

src/hash.rs line 104 at r10 (raw file):

What is the use case? Why serde implementation that is already in the types-rs is not enough?

src/block_hash/transaction_commitment.rs line 2 at r9 (raw file):

Done.

src/crypto/patricia_hash.rs line 29 at r9 (raw file):

Done.

src/crypto/utils.rs line 20 at r9 (raw file):

Frankly not sure, why it was needed.

Reviewed 18 of 18 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @LucasLvy, @maciejka, @ShahakShama, @tdelabro, and @Yoni-Starkware)

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @LucasLvy, @ShahakShama, @tdelabro, and @Yoni-Starkware)

src/core.rs line 358 at r3 (raw file):

Done.

src/external_transaction_test.rs line 33 at r8 (raw file):

Done

Reviewed 9 of 9 files at r13, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @LucasLvy, @maciejka, @ShahakShama, @tdelabro, and @Yoni-Starkware)

Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @LucasLvy, @ShahakShama, @tdelabro, and @Yoni-Starkware)

src/core.rs line 74 at r1 (raw file):

Done

src/hash.rs line 104 at r10 (raw file):

You mean you want felt! macro to be moved into types-rs?

src/crypto.rs line 19 at r5 (raw file):

Done

src/crypto.rs line 40 at r7 (raw file):

Done.

Reviewed all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @LucasLvy, @tdelabro, and @Yoni-Starkware)

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @LucasLvy, @tdelabro, and @Yoni-Starkware)

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @tdelabro and @Yoni-Starkware)

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @tdelabro and @Yoni-Starkware)

Reviewed 4 of 15 files at r1, 1 of 17 files at r8, 4 of 22 files at r9, 1 of 6 files at r10, 13 of 18 files at r11, 1 of 1 files at r12, 9 of 9 files at r13, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Yoni-Starkware)

Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @LucasLvy, @tdelabro, and @Yoni-Starkware)